Skip to content

Add -Zdirect-minimal-versions compatibility and testing. #7527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 25, 2025

Conversation

kpreid
Copy link
Collaborator

@kpreid kpreid commented Apr 12, 2025

Connections
Prevents problems like the one described in #7526.

Description
This PR adds a CI job which confirms building with cargo update -Zdirect-minimal-versions on some platforms. This unstable feature updates all dependencies from the workspace packages to non-workspace packages to the minimum version allowed by the workspace’s dependency specifications, thus ensuring that the project will not build if it actually depends on items newer than its dependency specifications.

It also updates dependency versions to be compatible with -Zdirect-minimal-versions. Specifically, cargo update -Zdirect-minimal-versions will fail if any transitive dependency has a higher dependency requirement than the workspace. This should largely make no difference since those versions would have been required anyway; however, it may increase the minimum dependency requirement seen by users of a single package from crates.io (e.g. users of naga without wgpu). That could be a reason to reject this change.

Also, this change will increase the Cargo.toml maintenance burden, because increasing some dependency versions may require also increasing others even when that change is a noop for any normal build.

Testing
I confirmed that tests pass with the dependencies actually updated, except for naga back::msl::writer::test_stack_size. However, the added CI job does not run tests, only cargo check, to minimize the additional CI cost. Thus, CI will catch use of newer items, but not bugs in old dependencies.

Squash or Rebase?
Rebase

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@kpreid kpreid requested a review from a team as a code owner April 12, 2025 22:51
@kpreid
Copy link
Collaborator Author

kpreid commented Apr 12, 2025

…whoops, I only just noticed that there is already a similar test for naga only. It takes a different approach and I'm not familiar with what cargo hack does there and I don't know if it is redundant.

@kpreid kpreid force-pushed the minvers branch 2 times, most recently from 0b03250 to 02746b0 Compare April 12, 2025 23:00
@cwfitzgerald
Copy link
Member

Oooo -Ztarget-minimal-version is super cool!

…whoops, I only just noticed that there is already a similar test for naga only. It takes a different approach and I'm not familiar with what cargo hack does there and I don't know if it is redundant.

This fully subsumes the naga test and it should be removed. cargo hack here is making the --remove-dev-deps option possible as there are issues with the dev deps with the normal minimal version flag.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super cool, one note, plus the naga check needs to be removed.

@cwfitzgerald
Copy link
Member

The mac version needs to be aarch64 - these are M1 runners.

@cwfitzgerald cwfitzgerald self-assigned this Apr 16, 2025
@kpreid
Copy link
Collaborator Author

kpreid commented Apr 25, 2025

I've redesigned the CI change so that instead of adding anything “new”, it just expands the coverage of the previous naga-minimal-versions job.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@cwfitzgerald cwfitzgerald merged commit b93b559 into gfx-rs:trunk Apr 25, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants